-
Notifications
You must be signed in to change notification settings - Fork 74
SDR replaces VectorHelpers #320
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
char is more space efficient, but our algorithms work with UInt for indices, so to avoid needed conversion. Also, internally mostly sdr_sparse_t is used for SDR data representation, so this should not have a significant impact on SDR
instead of VectorHelpers
as those are useful in algorithms working with SDRs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RFC on how to change SDR_dense_t? hard-code vector, or template?
src/examples/hotgym/HelloSPTP.cpp
Outdated
@@ -171,11 +170,11 @@ Real64 BenchmarkHotgym::run(UInt EPOCHS, bool useSPlocal, bool useSPglobal, bool | |||
|
|||
//Anomaly (pure x likelihood) | |||
tAn.start(); | |||
res = an.compute(outSP /*active*/, prevPred_ /*prev predicted*/); | |||
res = an.compute(outSP.getDense() /*active*/, prevPred_ /*prev predicted*/); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need SDR_dense_t to be vector UInt, not current vector Byte.
Should I set it just there.
Or make it templated?
It seems to be a big change which breaks a lot of stuff in SDR.
src/nupic/types/Sdr.hpp
Outdated
@@ -35,7 +35,7 @@ using namespace std; | |||
|
|||
namespace nupic { | |||
|
|||
typedef vector<Byte> SDR_dense_t; | |||
typedef vector<UInt> SDR_dense_t; //TODO add templated types for SDR<DenseElemT, SparseElemT, CoordElemT> + default SDR<Byte, UInt, UInt> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the breaking change. I think it'd be good to have this option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you saying that the SDR internal dense format should be UInt? That is not a good idea.
This reverts commit f651b42.
so SDR a = some_sdr; is now allowed. This is an ugly commit, too many changes. It mixes SDR moved under ns nupic::sdr; and some namespace fixes along the way. And need to replace &size, &dimensions with getter methods size(), dimensions() to allow the copy constructor which is needed for likes of: SDR a = sp.someFunctionReturningSDR();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not happy with the last, large commit, but I'd like to pull it through.
- ns nupic::sdr::SDR;
- SDR a = getsomeSDR(); //allows copy constructor
The SDR already allows the copy constructor...
|
This reverts commit d4cf24d.
this allows to avoid deepcopy and no need for copy-constructor for SDRs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this looks good.
Some questions about the changes to the SDR testcases though.
src/test/unit/types/SdrTest.cpp
Outdated
ASSERT_NE( before, after ); // not a copy. | ||
ASSERT_EQ( after, data ); // correct data buffer. | ||
} | ||
|
||
TEST(SdrTest, TestSetDenseByte) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where did this test case go?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought it was not needed due to some casts, but I'll revisit it again!
Btw, do we need all 3 variants for SDR::setXXX() ? Can we do just with SDR.setDense(SDR_dense_t)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The overloads are:
void setDense( SDR_dense_t &value );
void setDense( const std::vector<T> &value ) {
void setDense( const T *value ) {
constant & non-constant vectors need different code.
If vector of wrong data type it will do the converstion to Byte.
Raw pointer is for legacy code.
So yes, I think we need all of these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks better.
There are still a two issues with the SDR unit tests, where you've changed a data type. The SDR has a big matrix of different data types which should work, even though they are not the exact same type which the SDR uses internally.
Also, I don't see any changes to VectorHelpers. Isn't the purpose of this PR to remove it?
src/test/unit/types/SdrTest.cpp
Outdated
ASSERT_NE( before, after ); // not a copy. | ||
ASSERT_EQ( after, data ); // correct data buffer. | ||
} | ||
|
||
TEST(SdrTest, TestSetDenseUInt) { | ||
SDR a({11, 10, 4}); | ||
auto vec = SDR_dense_t(a.size, 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This data type should not have changed. The test name explicitly calls for UInt
not Byte
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SDR has a big matrix of different data types which should work, even though they are not the exact same type which the SDR uses internally.
yes, I made a misunderstood these. will need to get all of those back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I don't see any changes to VectorHelpers. Isn't the purpose of this PR to remove it?
yes, I've just updated the old PR to see how it fares. VectorH should be removed in this PR.
Also, I'm getting failing "exact output" tests but I don't see anything that should cause that (at the first glance)
@breznak, |
thanks for fixing those! |
@@ -548,8 +550,7 @@ vector<UInt> SpatialPooler::initMapPotential_(UInt column, bool wrapAround) { | |||
|
|||
const UInt numPotential = (UInt)round(columnInputs.size() * potentialPct_); | |||
const auto selectedInputs = rng_.sample<UInt>(columnInputs, numPotential); | |||
const vector<UInt> potential = VectorHelpers::sparseToBinary<UInt>(selectedInputs, numInputs_); | |||
return potential; | |||
potential.setSparse(selectedInputs); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not sorted! This is the bug causing the exactOutput
test to fail.
Because the potential pool is not sorted, the initial Permanences are randomly generated differently than before.
This also fails in debug mode where there is an ASSERT( is_sorted )
for this type of error.
most of the functionality now provided by SDR.hpp class
I want to replace some/most functionality of VectorHelpers by SDR.